pthread: fix key destructor calling and start function exit #304

badochov · 2023-10-19T11:01:22Z

Fixes phoenix-rtos/phoenix-rtos-project#675

Description

FIxes following posix imcompatibilities:

pthread_key_create

An optional destructor function may be associated with each key value. At thread exit, if a key value has a non-NULL destructor pointer, and the thread has a non-NULL value associated with that key, the value of the key is set to NULL, and then the function pointed to is called with the previously associated value as its sole argument. The order of destructor calls is unspecified if more than one destructor exists for a thread when it exits.

If, after all the destructors have been called for all non-NULL values with associated destructors, there are still some non-NULL values with associated destructors, then the process is repeated. If, after at least {PTHREAD_DESTRUCTOR_ITERATIONS} iterations of destructor calls for outstanding non-NULL values, there are still some non-NULL values with associated destructors, implementations may stop calling destructors, or they may continue calling destructors until no non-NULL values with associated destructors exist, even though this might result in an infinite loop.

pthread_key_delete

The pthread_key_delete() function shall be callable from within destructor functions. No destructor functions shall be invoked by pthread_key_delete(). Any destructor function that may have been associated with key shall no longer be called upon thread exit.

pthread_exit

An implicit call to pthread_exit() is made when a thread other than the thread in which main() was first invoked returns from the start routine that was used to create it. The function's return value shall serve as the thread's exit status.

pthread_detach

When a detached thread terminates, its
       resources are automatically released back to the system without
       the need for another thread to join with the terminated thread.

Also reworked how detached threads are handled

Code that previousle resoulted in faults:

#include <pthread.h>
#include <stdio.h>
#include <string.h>
#include <assert.h>

static pthread_key_t key1;
static pthread_key_t key2;

static pthread_once_t key_once = PTHREAD_ONCE_INIT;

static int deleted = 0;

static void dstrctr(void *data)
{
	/* Constructor of deleted key should not be called. */
	assert(data != deleted);

	/* should be printed only once */
	printf("DSTR %p\n", data);

	/* Order of destructor calls is not specified. */
	/* Delete other key. */
	if (data == (void*)1) {
		pthread_key_delete(key2);
		deleted = 1;
	} else {
		pthread_key_delete(key1);
		deleted = 2;
	}
}

void *fn(void *arg)
{
	(void)pthread_key_create(&key1, dstrctr);
	(void)pthread_key_create(&key2, dstrctr);

	pthread_setspecific(key1, (void *)1);
	pthread_setspecific(key2, (void *)2);


	pthread_exit(0); /* Old implementation had a bug where pthread_exit wasn't called on a return from function. */
}

int main(void)
{
	pthread_t thread;
	pthread_create(&thread, NULL, fn, NULL);
	pthread_join(thread, NULL);
	return 0;
}

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: ia-32

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

pthread/pthread.c

github-actions · 2023-10-19T11:14:58Z

Unit Test Results

5 957 tests +8 5 316 ✔️ +20 29m 9s ⏱️ - 2m 5s
  333 suites +8   641 💤 - 12
      1 files ±0       0 ❌ ±  0

Results for commit 47c37cb. ± Comparison against base commit bd3db90.

♻️ This comment has been updated with latest results.

pthread/pthread.c

agkaminski

Uh, lock releasing in functions make this code difficult, but I get it that it's necessary :)

pthread/pthread.c

agkaminski

LGTM

Destructor calling sequence at exit did not allow for key modifications in constructors. JIRA: RTOS-652

badochov · 2023-11-08T11:56:06Z

After lastest change of pthreads by @lukileczo in #309 cleanup pop push were added, I had to adjust the code to the existence of them.

pthread_exit was adjusted to comply with linux's interpretation of POSIX rules as they are very confusing:
POSIX pthread_exit excerpt:

An implicit call to pthread_exit() is made when a thread other than the thread in which main() was first invoked returns from the start routine that was used to create it. The function's return value shall serve as the thread's exit status.

POSIX pthread_cleanup_push excerpt:

The cancellation cleanup handler shall be popped from the cancellation cleanup stack and invoked with the argument arg when:

The thread exits (that is, calls [pthread_exit()](https://pubs.opengroup.org/onlinepubs/009696699/functions/pthread_exit.html)).

LINUX interpretation:

•  When a thread terminates by calling [pthread_exit(3)](https://man7.org/linux/man-pages/man3/pthread_exit.3.html), all
          clean-up handlers are executed as described in the preceding
          point.  (Clean-up handlers are not called if the thread
          terminates by performing a return from the thread start
          function.)

Fixed cleanup handlers not being called in case of cancelling not self thread

pthread/pthread.c

pthread_exit fix destroying ctx on JOINABLE thread. Rework handling of detached threads. JIRA: RTOS-654

badochov · 2023-11-08T12:44:47Z

Also I spotted that signal_cancel is send only after everything is cleanup it seems racy to. Is this valid @agkaminski ?

libphoenix/pthread/pthread.c

Line 433 in bd3db90

err = signalPost(getpid(), id, signal_cancel);

lukileczo · 2023-11-08T12:51:38Z

signal_cancel calls proc_threadDestroy immediately in kernel on the requested thread, so all resources should be freed before. Our cancel implementation is not fully POSIX compliant, as we have no cancellation points (which would require many changes across libphoenix)

lukileczo

LGTM

agkaminski

Clang-format stuff nitpick, otherwise LGTM

agkaminski · 2023-11-08T15:15:30Z

pthread/pthread.c

@@ -556,7 +590,7 @@ int pthread_attr_setschedparam(pthread_attr_t *attr,
 		return EINVAL;

 	if (param->sched_priority > sched_get_priority_max(SCHED_RR) ||
-			param->sched_priority < sched_get_priority_min(SCHED_RR))
+		param->sched_priority < sched_get_priority_min(SCHED_RR))


clang-format is mistaken ;)

agkaminski · 2023-11-08T15:15:47Z

pthread/pthread.c

-	if (detachstate != PTHREAD_CREATE_DETACHED ||
-			detachstate != PTHREAD_CREATE_JOINABLE)
+	if ((detachstate != PTHREAD_CREATE_DETACHED) ||
+		(detachstate != PTHREAD_CREATE_JOINABLE)) {


clang-format is mistaken ;)

JIRA: RTOS-671

github-actions · 2023-11-08T16:59:28Z

pthread/pthread.c

-	if (detachstate != PTHREAD_CREATE_DETACHED ||
-			detachstate != PTHREAD_CREATE_JOINABLE)
+	if ((detachstate != PTHREAD_CREATE_DETACHED) ||
+			(detachstate != PTHREAD_CREATE_JOINABLE)) {


[clang-format-pr] _{reported by reviewdog 🐶}
suggested fix

Suggested change

(detachstate != PTHREAD_CREATE_JOINABLE)) {

(detachstate != PTHREAD_CREATE_JOINABLE)) {

github-actions bot reviewed Oct 19, 2023

View reviewed changes

pthread/pthread.c Show resolved Hide resolved

badochov force-pushed the badochov/pthread_key_cleanup branch from 3b8b89c to 50f6736 Compare October 19, 2023 11:02

badochov marked this pull request as draft October 19, 2023 11:11

badochov force-pushed the badochov/pthread_key_cleanup branch 2 times, most recently from 864fa71 to ad87dee Compare October 24, 2023 16:16

github-actions bot reviewed Oct 24, 2023

View reviewed changes

pthread/pthread.c Outdated Show resolved Hide resolved

badochov force-pushed the badochov/pthread_key_cleanup branch from ad87dee to 903161c Compare October 27, 2023 14:59

badochov requested review from adamdebek and agkaminski October 27, 2023 15:05

badochov marked this pull request as ready for review October 27, 2023 15:05

badochov force-pushed the badochov/pthread_key_cleanup branch 2 times, most recently from 1755420 to 96c6e84 Compare October 27, 2023 15:30

agkaminski reviewed Nov 2, 2023

View reviewed changes

pthread/pthread.c Outdated Show resolved Hide resolved

pthread/pthread.c Outdated Show resolved Hide resolved

badochov force-pushed the badochov/pthread_key_cleanup branch from 96c6e84 to 9ddde65 Compare November 2, 2023 13:57

badochov requested a review from agkaminski November 2, 2023 13:57

agkaminski previously approved these changes Nov 2, 2023

View reviewed changes

pthread: rework how destructors are called

d1c213a

Destructor calling sequence at exit did not allow for key modifications in constructors. JIRA: RTOS-652

badochov dismissed agkaminski’s stale review via 8dea15d November 8, 2023 11:47

badochov force-pushed the badochov/pthread_key_cleanup branch from 9ddde65 to 8dea15d Compare November 8, 2023 11:47

badochov requested a review from lukileczo November 8, 2023 11:56

badochov force-pushed the badochov/pthread_key_cleanup branch from 8dea15d to 6b032ae Compare November 8, 2023 12:04

lukileczo reviewed Nov 8, 2023

View reviewed changes

pthread/pthread.c Outdated Show resolved Hide resolved

phtread: call pthread_exit implicitely on return from start function

271ebd2

pthread_exit fix destroying ctx on JOINABLE thread. Rework handling of detached threads. JIRA: RTOS-654

badochov force-pushed the badochov/pthread_key_cleanup branch from 6b032ae to 87e41cb Compare November 8, 2023 12:42

lukileczo previously approved these changes Nov 8, 2023

View reviewed changes

agkaminski reviewed Nov 8, 2023

View reviewed changes

pthread: destroy cleanup handlers on cancel

47c37cb

JIRA: RTOS-671

badochov dismissed lukileczo’s stale review via 47c37cb November 8, 2023 16:59

badochov force-pushed the badochov/pthread_key_cleanup branch from 87e41cb to 47c37cb Compare November 8, 2023 16:59

github-actions bot reviewed Nov 8, 2023

View reviewed changes

agkaminski approved these changes Nov 8, 2023

View reviewed changes

agkaminski merged commit 090a2fd into master Nov 9, 2023
29 of 30 checks passed

agkaminski deleted the badochov/pthread_key_cleanup branch November 9, 2023 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pthread: fix key destructor calling and start function exit #304

pthread: fix key destructor calling and start function exit #304

badochov commented Oct 19, 2023 •

edited

Loading

github-actions bot commented Oct 19, 2023 •

edited

Loading

agkaminski left a comment

agkaminski left a comment

badochov commented Nov 8, 2023 •

edited

Loading

badochov commented Nov 8, 2023

lukileczo commented Nov 8, 2023 •

edited

Loading

lukileczo left a comment

agkaminski left a comment

agkaminski Nov 8, 2023

agkaminski Nov 8, 2023

github-actions bot Nov 8, 2023

	(detachstate != PTHREAD_CREATE_JOINABLE)) {
	(detachstate != PTHREAD_CREATE_JOINABLE)) {

pthread: fix key destructor calling and start function exit #304

pthread: fix key destructor calling and start function exit #304

Conversation

badochov commented Oct 19, 2023 • edited Loading

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

github-actions bot commented Oct 19, 2023 • edited Loading

Unit Test Results

agkaminski left a comment

Choose a reason for hiding this comment

agkaminski left a comment

Choose a reason for hiding this comment

badochov commented Nov 8, 2023 • edited Loading

badochov commented Nov 8, 2023

lukileczo commented Nov 8, 2023 • edited Loading

lukileczo left a comment

Choose a reason for hiding this comment

agkaminski left a comment

Choose a reason for hiding this comment

agkaminski Nov 8, 2023

Choose a reason for hiding this comment

agkaminski Nov 8, 2023

Choose a reason for hiding this comment

github-actions bot Nov 8, 2023

Choose a reason for hiding this comment

badochov commented Oct 19, 2023 •

edited

Loading

github-actions bot commented Oct 19, 2023 •

edited

Loading

badochov commented Nov 8, 2023 •

edited

Loading

lukileczo commented Nov 8, 2023 •

edited

Loading